Skip to content

Fix Ord codegen to use other < self in reverse branch#170

Merged
nunoplopes merged 2 commits into
masterfrom
copilot/fix-ord-comparison-issue
Jun 3, 2026
Merged

Fix Ord codegen to use other < self in reverse branch#170
nunoplopes merged 2 commits into
masterfrom
copilot/fix-ord-comparison-issue

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Jun 3, 2026

The generated Ord::cmp for method-based operator< used other.<op>(other) in its second branch, which can never evaluate true and therefore suppresses Ordering::Greater. This change aligns that branch with the intended reverse comparison (other.<op>(self)), matching existing model-specific behavior.

  • Codegen fix (converter.cpp)

    • In Converter::ConvertOrdAndPartialOrdTraits (OO_Less + method operator path), changed:
      • second_branch = other.<op>(other)second_branch = other.<op>(self)
  • Golden output update (unsafe unit fixture)

    • Updated tests/unit/out/unsafe/operator_less_than.rs to reflect corrected generation in both:
      • Ord::cmp reverse branch
      • PartialEq::eq reverse comparison branch
// before
second_branch = std::format("other.{}(other)", GetOverloadedOperator(op));

// after
second_branch = std::format("other.{}(self)", GetOverloadedOperator(op));
Original prompt
Please apply the following diffs and create a pull request.
Once the PR is ready, give it a title based on the messages of the fixes being applied.

[{"message":"The second branch of the `Ord` comparison incorrectly compares `other` against itself (`other.lt(other)`) instead of comparing `other` against `self` (`other.lt(self)`). This means the generated `cmp` method will never produce `std::cmp::Ordering::Greater`, since `other < other` is always false. The `ConverterRefCount` override at line 418 correctly uses `self` in the equivalent position. This argument should be `self`, not `other`.","fixFiles":[{"filePath":"cpp2rust/converter/converter.cpp","diff":"diff --git a/cpp2rust/converter/converter.cpp b/cpp2rust/converter/converter.cpp\n--- a/cpp2rust/converter/converter.cpp\n+++ b/cpp2rust/converter/converter.cpp\n@@ -3678,7 +3678,7 @@\n   case clang::OO_Less:\n     if (clang::isa<clang::CXXMethodDecl>(op)) {\n       first_branch = std::format(\"self.{}(other)\", GetOverloadedOperator(op));\n-      second_branch = std::format(\"other.{}(other)\", GetOverloadedOperator(op));\n+      second_branch = std::format(\"other.{}(self)\", GetOverloadedOperator(op));\n     } else {\n       first_branch = std::format(\"{}(self, other)\", GetOverloadedOperator(op));\n       second_branch = std::format(\"{}(other, self)\", GetOverloadedOperator(op));\n"}]}]

Copilot AI changed the title [WIP] Fix incorrect comparison in Ord implementation Fix Ord codegen to use other < self in reverse branch Jun 3, 2026
Copilot AI requested a review from nunoplopes June 3, 2026 14:13
@nunoplopes nunoplopes marked this pull request as ready for review June 3, 2026 14:14
@nunoplopes nunoplopes merged commit 5be4f50 into master Jun 3, 2026
9 checks passed
@nunoplopes nunoplopes deleted the copilot/fix-ord-comparison-issue branch June 3, 2026 14:19
lucic71 pushed a commit to lucic71/cpp2rust that referenced this pull request Jun 4, 2026
The generated `Ord::cmp` for method-based `operator<` used
`other.<op>(other)` in its second branch, which can never evaluate true
and therefore suppresses `Ordering::Greater`. This change aligns that
branch with the intended reverse comparison (`other.<op>(self)`),
matching existing model-specific behavior.

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants